Fix broken caching system when array of allowed parameters used #6410

JavaDeveloperKiev · 2022-08-22T11:09:28Z

Cache Include Query String in the /Config/Cache.php (line 73) suggest to the user 3 options to choose from:

false = Disabled

true = Enabled, take all query parameters into account. Please be aware that this may result in numerous cache files generated for the same page over and over again.

array('q') = Enabled, but only take into account the specified list of query parameters.

However, last option with array of important parameters just doesn't work. We were using third option, because we had one important parameter (color) on the site, and content were dynamic based on this parameter.

But then we`ve seen huge amount of cache files on our server, thousands and thousands. Turned out, even when using array of parameters in Config/Cache $cacheQueryString option - CodeIgniter was creating new cache file for any requests just like it was set to 'true'. Cache fles generated for the same page over and over again, performance were slow.

Since Google Ads, Bing, Yahoo and all marketing platforms always adding dynamic parameters like 'click_id' for example, 'utm_source', 'utm_medium' and other, cache file was creating for all requests from the paid traffic. It was almost impossible to catch, but we saw weird amount of caching files, and that helped us to see the problem.

I`ve made testing on the clean CodeIgniter 4 installations, latest version. Happened there too.

So I`ve checked the source code, and I saw there there is no difference between true/array options, they just work same. I believe this is very missleading, because Config/Cache showing 'array' option and suggest to use it.

This fixes should help, and take into the account array option.

Thank you

Checklist:

Securely signed commits
Component(s) with PHPDoc blocks, only if necessary or adds value
Unit testing, with >80% coverage
User guide updated
Conforms to style guide

…judge if validation fields have asterisk

Co-authored-by: kenjis <[email protected]>

…ion used Cache Include Query String in the /Config/Cache.php (line 73) suggest to the user 3 options to choose from: false = Disabled true = Enabled, take all query parameters into account. Please be aware that this may result in numerous cache files generated for the same page over and over again. array('q') = Enabled, but only take into account the specified list of query parameters. However, last option with array of important parameters just doesn't work. We were using third option, because we had one important parameter (color) on the site, and content were dynamic based on this parameter. But then we`ve seen huge amount of cache files on our server, thousands and thousands. Turned out, even when using array of parameters in Config/Cache $cacheQueryString option - CodeIgniter was creating new cache file for any requests just like it was set to 'true'. Cache fles generated for the same page over and over again, performance were slow. Since Google Ads, Bing, Yahoo and all marketing platforms always adding dynamic parameters like 'click_id' for example, 'utm_source', 'utm_medium' and other, cache file was creating for all requests from the paid traffic. It was almost impossible to catch, but we saw weird amount of caching files, and that helped us to see the problem. I`ve made testing on the clean CodeIgniter 4 installations, latest version. Happened there too. So I`ve checked the source code, and I saw there there is no difference between true/array options, theyjust work same. I believe this is very missleading, because Config/Cache showing 'array' option and suggest to use it. This fixes should help, and take into the account array option. Thank you

docs : fix typo in "General Topics » Managing your Applications"

…tom-error-asterisk-field Fix validation custom error asterisk field

Exclude non-monolithic files from CS

kenjis · 2022-08-23T08:27:45Z

CodeIgniter4/app/Config/Cache.php

Lines 55 to 71 in 9546098

    
                * -------------------------------------------------------------------------- 
        
                * Cache Include Query String 
        
                * -------------------------------------------------------------------------- 
        
                * 
        
                * Whether to take the URL query string into consideration when generating 
        
                * output cache files. Valid options are: 
        
                * 
        
                *    false      = Disabled 
        
                *    true       = Enabled, take all query parameters into account. 
        
                *                 Please be aware that this may result in numerous cache 
        
                *                 files generated for the same page over and over again. 
        
                *    array('q') = Enabled, but only take into account the specified list 
        
                *                 of query parameters. 
        
                * 
        
                * @var bool|string[] 
        
                */ 
        
               public $cacheQueryString = false;

kenjis · 2022-08-23T08:38:34Z

Thank you for sending a PR!

Please fix coding style. See GitHub Action checks.

Please add test code.

https://github.com/codeigniter4/CodeIgniter4/tree/develop/tests#resources

we expect all code changes or bug-fixes to be accompanied by one or more tests added to our test suite to prove the code works. If pull requests are not accompanied by relevant tests, they will likely be closed. Since we are a team of volunteers, we don't have any more time to work on the framework than you do. Please make it as painless for your contributions to be included as possible. If you need help with getting tests running on your local machines, ask for help on the forums. We would be happy to help out.
https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/pull_request.md#contributions

It is better to use dedicated database for testing.

JavaDeveloperKiev · 2022-08-23T11:14:49Z

kenjis

Sure thing :)

I am going to write a test, but method that generating cache name (and having the bug) is protected.

What is the best practice in CodeIgniter 4, to create a test for protected methods? Should I change it to public to run a test, or do something else?

Thanks!

kenjis · 2022-08-23T11:38:09Z

Sorry, it is difficult to write test code for it.

What is the best practice in CodeIgniter 4, to create a test for protected methods?

It is fine in this case,

Or integration test like this:

CodeIgniter4/tests/system/CodeIgniterTest.php

Line 569 in 9546098

public function testPageCacheSendSecureHeaders()

Should I change it to public to run a test

No. We cannot change public APIs for bug fixes (as much as possible).

Three new tests, for all suggested options of $cacheQueryString in the Config/Cache.php cacheQueryString = true cacheQueryString = false cacheQueryString = array('q')

JavaDeveloperKiev · 2022-08-23T15:50:32Z

Sorry, it is difficult to write test code for it.

What is the best practice in CodeIgniter 4, to create a test for protected methods?

It is fine in this case,

Or integration test like this:

CodeIgniter4/tests/system/CodeIgniterTest.php

Line 569 in 9546098

public function testPageCacheSendSecureHeaders()

Should I change it to public to run a test

No. We cannot change public APIs for bug fixes (as much as possible).

Okay, I`ve created three new tests, for all possible values of the $cacheQueryString in Config/Cache.php

Also the bug is fixed in CodeIgniter.php, and now when $cacheQueryString is set to array of important parameters - it works how it should.

If you run tests in the current dev/master branch, you`ll see that testPageCacheWithCacheQueryStringArray() test is failling, because CodeIgniter creating 5 cache files instead of 3 expected.

Thats pretty critical bug, because its generating thousands of cache files in the production server, and showing not cached version of the pages for all users with unique not important parameters like utm_source/click_id/etc

In my branch its fixed

kenjis · 2022-08-30T08:37:40Z

Can you rebase?

…espace docs: add about controllers namespace in routing

locale Is a BCP 47 compliant language tag. See https://www.php.net/manual/en/locale.setdefault.php

docs: update PHPDoc in Entity

…ales test: restore changed locale in Time tests

…ion used Cache Include Query String in the /Config/Cache.php (line 73) suggest to the user 3 options to choose from: false = Disabled true = Enabled, take all query parameters into account. Please be aware that this may result in numerous cache files generated for the same page over and over again. array('q') = Enabled, but only take into account the specified list of query parameters. However, last option with array of important parameters just doesn't work. We were using third option, because we had one important parameter (color) on the site, and content were dynamic based on this parameter. But then we`ve seen huge amount of cache files on our server, thousands and thousands. Turned out, even when using array of parameters in Config/Cache $cacheQueryString option - CodeIgniter was creating new cache file for any requests just like it was set to 'true'. Cache fles generated for the same page over and over again, performance were slow. Since Google Ads, Bing, Yahoo and all marketing platforms always adding dynamic parameters like 'click_id' for example, 'utm_source', 'utm_medium' and other, cache file was creating for all requests from the paid traffic. It was almost impossible to catch, but we saw weird amount of caching files, and that helped us to see the problem. I`ve made testing on the clean CodeIgniter 4 installations, latest version. Happened there too. So I`ve checked the source code, and I saw there there is no difference between true/array options, theyjust work same. I believe this is very missleading, because Config/Cache showing 'array' option and suggest to use it. This fixes should help, and take into the account array option. Thank you

Three new tests, for all suggested options of $cacheQueryString in the Config/Cache.php cacheQueryString = true cacheQueryString = false cacheQueryString = array('q')

Using data provider for 1 cache test instead of 3 different tests. https://phpunit.readthedocs.io/en/9.5/writing-tests-for-phpunit.html#writing-tests-for-phpunit-data-providers

…type to strict type for the

…niter4 into patch-1

JavaDeveloperKiev · 2022-09-01T19:01:10Z

Can you rebase?

Sure, rebased branch, now all commits should be verified

kenjis · 2022-09-01T21:19:31Z

Oh, now this PR has 108 commits!
What command did you run?

How to rebase:
https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/workflow.md#updating-your-branch

JavaDeveloperKiev · 2022-09-02T07:23:06Z

Oh, now this PR has 108 commits! What command did you run?

How to rebase: https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/workflow.md#updating-your-branch

Not sure what went wrong, I copied commands from instructions, see the snapshot. As I understand, its merged all existing commits from upstream/develop to this branch in last 10 days, and then added mine commits over it.

Want to me create new branch and open another PR instead of this one?

kenjis · 2022-09-02T08:38:05Z

Want to me create new branch and open another PR instead of this one?

Yes.
I cannot merge this PR because there are commits that are not related to this fix.
It seems easier to create new branch and open another PR.

JavaDeveloperKiev · 2022-09-02T09:50:37Z

Want to me create new branch and open another PR instead of this one?

Yes. I cannot merge this PR because there are commits that are not related to this fix. It seems easier to create new branch and open another PR.

I have created a new branch based on latest origin/dev to make a clean PR, but now when I try to commit I`m getting errors from the PHP Stan, not related to my changes? Any idea?

JavaDeveloperKiev · 2022-09-02T09:53:11Z

Same errors in GitHub desktop, not related to my changes

JavaDeveloperKiev · 2022-09-02T09:59:33Z

I`ve double checked, my new branch is up to date with /dev

kenjis · 2022-09-02T12:17:24Z

I have checked the latest develop.

$ vendor/bin/phpstan
Note: Using configuration file /Users/kenji/work/codeigniter/CodeIgniter4/phpstan.neon.dist.
 434/434 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%


                                                                                                                        
 [OK] No errors

Your origin/develop is up to date?

See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/workflow.md#updating-your-branch

kenjis · 2022-09-02T12:18:37Z

If you don't update Composer dependencies, run composer update.

JavaDeveloperKiev · 2022-09-02T12:30:52Z

If you don't update Composer dependencies, run composer update.

Worked like a charm, thank you!

Please check new PR:
#6475

ping-yee and others added 14 commits August 20, 2022 01:17

fix: add a new param in processRules and getErrorMessage function to …

152767d

…judge if validation fields have asterisk

add : add a description to the changelog in v4.2.5.

9888bba

changelog: change changelog

574feb7

test: add test for validation error with asterisk field.

964fe71

Update Table.php

8b4b16a

test: refactor tests for getFieldData()

1cc0f9b

Exclude non-monolithic files from CS

e48296e

Update system/Database/SQLite3/Table.php

66af822

Co-authored-by: kenjis <[email protected]>

docs : fix typo in "General Topics » Managing your Applications"

80eb987

Merge pull request codeigniter4#6411 from cijagani/develop

40c69f0

docs : fix typo in "General Topics » Managing your Applications"

fix: cs-fix

512f253

Merge pull request codeigniter4#6378 from ping-yee/fix-validation-cus…

071cd04

…tom-error-asterisk-field Fix validation custom error asterisk field

Merge pull request codeigniter4#6401 from paulbalandan/cs-exclude

9546098

Exclude non-monolithic files from CS

kenjis added bug Verified issues on the current code behavior or pull requests that will fix them tests needed Pull requests that need tests labels Aug 23, 2022

kenjis and others added 4 commits August 23, 2022 18:05

docs: update about test database

1760b09

docs: update test database config sample in .env

7100bc6

config: change commented database.tests.database value

fa474ad

It is better to use dedicated database for testing.

Fix coding style

a04b27d

docs: restore phpunit.xml

0fc684e

Created 3 tests for caching with different cacheQueryString options

7b4f482

Three new tests, for all suggested options of $cacheQueryString in the Config/Cache.php cacheQueryString = true cacheQueryString = false cacheQueryString = array('q')

JavaDeveloperKiev and others added 3 commits August 23, 2022 20:47

Refactoring syntax edits to pass Rector 7.4 and 8.0 check

8adacb0

Add testDropColumnDropCompositeKey()

b2ad447

Fix test

7717097

Update FileRules.php

426873f

MGatner and others added 8 commits August 30, 2022 06:40

Merge pull request codeigniter4#6452 from daycry/patch-1

cd4bc83

docs: improve sample code

e046890

Merge pull request codeigniter4#6451 from kenjis/fix-docs-routing-nam…

a51b07c

…espace docs: add about controllers namespace in routing

test: restore changed locale in tearDown()

1aeb5b3

docs: update PHPDoc in Entity

69c407b

test: fix invalid locale value

528eed7

locale Is a BCP 47 compliant language tag. See https://www.php.net/manual/en/locale.setdefault.php

Merge pull request codeigniter4#6462 from kenjis/fix-Entity-phpdoc

5b396db

docs: update PHPDoc in Entity

Merge pull request codeigniter4#6460 from kenjis/fix-test-restore-loc…

57385ea

…ales test: restore changed locale in Time tests

kenjis added the waiting for info Issues or pull requests that need further clarification from the author label Sep 1, 2022

JavaDeveloperKiev added 7 commits September 1, 2022 21:57

Fix coding style

6460513

Created 3 tests for caching with different cacheQueryString options

903b78d

Three new tests, for all suggested options of $cacheQueryString in the Config/Cache.php cacheQueryString = true cacheQueryString = false cacheQueryString = array('q')

Refactoring syntax edits to pass Rector 7.4 and 8.0 check

2a4a990

Updating the Cache test to use data provider

5237e30

Using data provider for 1 cache test instead of 3 different tests. https://phpunit.readthedocs.io/en/9.5/writing-tests-for-phpunit.html#writing-tests-for-phpunit-data-providers

Use parameter type declaration instead of doc comment | Change mixed …

48d2dc6

…type to strict type for the

Merge branch 'patch-1' of https://github.com/JavaDeveloperKiev/CodeIg…

512d9ae

…niter4 into patch-1

JavaDeveloperKiev mentioned this pull request Sep 2, 2022

Fix broken caching system when array of allowed parameters used #6475

Merged

5 tasks

kenjis closed this Sep 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix broken caching system when array of allowed parameters used #6410

Fix broken caching system when array of allowed parameters used #6410

JavaDeveloperKiev commented Aug 22, 2022 •

edited by kenjis

Loading

kenjis commented Aug 23, 2022

kenjis commented Aug 23, 2022

JavaDeveloperKiev commented Aug 23, 2022

kenjis commented Aug 23, 2022

JavaDeveloperKiev commented Aug 23, 2022

kenjis commented Aug 30, 2022

JavaDeveloperKiev commented Sep 1, 2022

kenjis commented Sep 1, 2022 •

edited

Loading

JavaDeveloperKiev commented Sep 2, 2022

kenjis commented Sep 2, 2022

JavaDeveloperKiev commented Sep 2, 2022 •

edited

Loading

JavaDeveloperKiev commented Sep 2, 2022

JavaDeveloperKiev commented Sep 2, 2022

kenjis commented Sep 2, 2022

kenjis commented Sep 2, 2022

JavaDeveloperKiev commented Sep 2, 2022

Fix broken caching system when array of allowed parameters used #6410

Fix broken caching system when array of allowed parameters used #6410

Conversation

JavaDeveloperKiev commented Aug 22, 2022 • edited by kenjis Loading

kenjis commented Aug 23, 2022

kenjis commented Aug 23, 2022

JavaDeveloperKiev commented Aug 23, 2022

kenjis commented Aug 23, 2022

JavaDeveloperKiev commented Aug 23, 2022

kenjis commented Aug 30, 2022

JavaDeveloperKiev commented Sep 1, 2022

kenjis commented Sep 1, 2022 • edited Loading

JavaDeveloperKiev commented Sep 2, 2022

kenjis commented Sep 2, 2022

JavaDeveloperKiev commented Sep 2, 2022 • edited Loading

JavaDeveloperKiev commented Sep 2, 2022

JavaDeveloperKiev commented Sep 2, 2022

kenjis commented Sep 2, 2022

kenjis commented Sep 2, 2022

JavaDeveloperKiev commented Sep 2, 2022

JavaDeveloperKiev commented Aug 22, 2022 •

edited by kenjis

Loading

kenjis commented Sep 1, 2022 •

edited

Loading

JavaDeveloperKiev commented Sep 2, 2022 •

edited

Loading